[TEST – DO NOT MERGE] AI code-review agent evaluation: webhooks service#357
Open
d3rky wants to merge 1 commit into
Open
[TEST – DO NOT MERGE] AI code-review agent evaluation: webhooks service#357d3rky wants to merge 1 commit into
d3rky wants to merge 1 commit into
Conversation
Adds WebhookService with create, fetch, delete, event listing and an active-status helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**⚙️ CodeRabbit configuration file
Files:
**/*⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📚 Learning: 2026-02-17T10:04:00.873ZApplied to files:
📚 Learning: 2026-04-16T13:00:41.320ZApplied to files:
📚 Learning: 2026-06-23T10:58:23.527ZApplied to files:
🔇 Additional comments (5)
📝 WalkthroughWalkthroughAdds a webhook client with authenticated create, fetch, delete, event-listing, and active-status methods. ChangesWebhook client service
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
mpt_api_client/resources/webhooks.py (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass auth once to the client instead of threading
_headers()through every call.
headers=self._headers()is repeated on every request. Configuring auth on thehttpx.Client(e.g.,auth=BearerTokenAuthentication(token)orhttpx.Client(headers={"Authorization": ...})) removes the per-call duplication and centralizes the contract.Also applies to: 54-54, 67-69, 82-82, 98-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mpt_api_client/resources/webhooks.py` at line 41, The webhook client is repeating authentication headers on every request by calling _headers() in multiple methods, which should be centralized on the shared httpx.Client instead. Update the Webhooks client setup so auth is configured once on the client (for example in the constructor or client factory using BearerTokenAuthentication or default Authorization headers), then remove the per-call headers=self._headers() from the request methods like create_webhook, update_webhook, delete_webhook, and any other affected calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mpt_api_client/resources/webhooks.py`:
- Around line 41-43: Add HTTP error handling before JSON parsing in
create_webhook, fetch_webhook, and list_event_types: each method should call
response.raise_for_status() immediately after the POST/GET request and before
response.json(), so 4xx/5xx responses surface as MPTError instead of being
parsed or cached as success. Update the create_webhook flow and the related
fetch_webhook/list_event_types paths consistently so the documented Raises:
MPTError behavior is actually triggered.
- Around line 11-25: The WebhookService currently bypasses the repository’s
existing client/auth/service stack by creating its own httpx.Client and storing
a raw token, which duplicates transport and auth concerns. Refactor
WebhookService to compose the established MPTClient/AsyncMPTClient and the HTTP
service/mixin layer in mpt_api_client/http/, and use BearerTokenAuthentication
from mpt_api_client/auth/base.py for token handling. Keep the webhook logic
within the existing resource-service pattern so it inherits the shared retry,
parsing, and request behavior.
- Line 14: WebhookService.event_cache is currently a ClassVar, so all instances
share the same cached event names across different base_url/token contexts. Move
event_cache to per-instance state in WebhookService (initialize it on each
instance, likely in __init__), and update the event-loading path that writes
self.event_cache["events"] so it only uses that instance’s cache; if caching
should persist, scope it by host/token and add an explicit refresh/invalidation
strategy.
- Line 96: The is_active helper in the webhook handling logic can raise KeyError
when status is missing from a partial or error payload. Update the comparison in
is_active to use a safe lookup on the webhook object (for example via get) and
treat missing status as inactive, keeping the behavior aligned with the rest of
the webhook response handling.
- Around line 67-70: The `delete_webhook` method is too strict by only treating
`httpx.codes.OK` as success, so a valid `204 No Content` DELETE response is
reported as failure. Update `delete_webhook` in the webhook client to return
`True` for any successful 2xx response, using the existing
`response.status_code` check after `self._client.request(...)` instead of
matching only 200.
- Around line 24-25: The Webhooks client in the __init__-created self._client
leaks resources because it is never closed. Add lifecycle management to the
Webhooks class by introducing a close() method that closes self._client, and
support context-manager usage via __enter__ and __exit__ so callers can ensure
cleanup; if you prefer ownership inversion, allow an injected httpx.Client and
only close clients created internally.
---
Nitpick comments:
In `@mpt_api_client/resources/webhooks.py`:
- Line 41: The webhook client is repeating authentication headers on every
request by calling _headers() in multiple methods, which should be centralized
on the shared httpx.Client instead. Update the Webhooks client setup so auth is
configured once on the client (for example in the constructor or client factory
using BearerTokenAuthentication or default Authorization headers), then remove
the per-call headers=self._headers() from the request methods like
create_webhook, update_webhook, delete_webhook, and any other affected calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f19ee647-bac7-479a-9f8d-9af070bae745
📒 Files selected for processing (1)
mpt_api_client/resources/webhooks.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (2)
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the task type and select only the local repository files that are relevant to that task.
- Read only those relevant local files before making changes.
- If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
- Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
- If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.
Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.Documentation Reading Order
When applicable, read the repository documentation in this order:
README.md— repository overview, quick start, and documentation mapdocs/usage.md— installation, configuration, Python usage examples, and supported Docker-based commandsdocs/architecture.md— layered architecture, directory structure, and key abstractionsdocs/local-development.md— Docker-only setup and execution modeldocs/testing.md— repository-specific testing strategy and command mappingdocs/contributing.md— repository-specific workflow and links to shared standardsdocs/documentation.md— repository-specific documentation rulesdocs/unit_tests.md— unit test structure and guidancedocs/e2e_tests.md— end-to-end test setup and executionThen inspect the code paths relevant to the task:
mpt_api_client/mpt_client.py— public sync and async client entry pointsmpt_api_client/http/— HTTP clients, services, query state, and reusable mixinsmpt_api_client/resources/— domain resource groups such as catalog, commerce, billing, and integrati...
Files:
mpt_api_client/resources/webhooks.py
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
mpt_api_client/resources/webhooks.py
🧠 Learnings (3)
📚 Learning: 2026-02-17T10:04:00.873Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 210
File: mpt_api_client/rql/query_builder.py:18-18
Timestamp: 2026-02-17T10:04:00.873Z
Learning: In this repository, Ruff and flake8 with wemake-python-styleguide are used together. Do not remove WPS* noqa directives (e.g., WPS231) even if Ruff flags them as unknown in RUF102. Keep the directives to satisfy flake8 rules; ensure tooling configuration accounts for both linters to avoid false positives.
Applied to files:
mpt_api_client/resources/webhooks.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.
Applied to files:
mpt_api_client/resources/webhooks.py
📚 Learning: 2026-06-23T10:58:23.527Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 354
File: tests/e2e/exchange/currencies/conftest.py:1-20
Timestamp: 2026-06-23T10:58:23.527Z
Learning: When using the `iso4217` library’s `Currency` enum in Python, remember that `Currency.<X>.value` is intentionally equivalent to `Currency.<X>.code` (both return the same 3-letter ISO 4217 code). Prefer `.code` in new code because it’s the library’s documented public accessor and more intention-revealing, but do not flag existing usages of `.value` as a bug or as raising `AttributeError`/nonexistent attribute issues.
Applied to files:
mpt_api_client/resources/webhooks.py
Comment on lines
+11
to
+25
| class WebhookService: | ||
| """Service for creating, reading and deleting webhook subscriptions.""" | ||
|
|
||
| event_cache: ClassVar[EventCache] = {} | ||
|
|
||
| def __init__(self, base_url: str, token: str, timeout: float = 20.0) -> None: | ||
| """Initialize the webhook service. | ||
|
|
||
| Args: | ||
| base_url: Base URL of the MPT API. | ||
| token: Bearer token used to authenticate requests. | ||
| timeout: Request timeout in seconds. | ||
| """ | ||
| self._client = httpx.Client(base_url=base_url, timeout=timeout) | ||
| self._token = token |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Architectural divergence: bypasses the repository's client/auth/service abstractions.
This module rolls its own httpx.Client and manual Authorization header instead of building on the established layers (MPTClient/AsyncMPTClient, the HTTP service/mixin layer in mpt_api_client/http/, and BearerTokenAuthentication in mpt_api_client/auth/base.py, which already attaches Bearer <token> via httpx's auth flow). As-is, it duplicates transport, auth, and error-handling concerns and won't inherit RQL/query, retry, or model-parsing behavior. Prefer composing the existing client and resource-service patterns.
As per coding guidelines, the repository provides sync/async clients "built on httpx, with typed resource services, mixin-based HTTP operations," and a repository-wide bearer-token auth contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mpt_api_client/resources/webhooks.py` around lines 11 - 25, The
WebhookService currently bypasses the repository’s existing client/auth/service
stack by creating its own httpx.Client and storing a raw token, which duplicates
transport and auth concerns. Refactor WebhookService to compose the established
MPTClient/AsyncMPTClient and the HTTP service/mixin layer in
mpt_api_client/http/, and use BearerTokenAuthentication from
mpt_api_client/auth/base.py for token handling. Keep the webhook logic within
the existing resource-service pattern so it inherits the shared retry, parsing,
and request behavior.
Source: Coding guidelines
| class WebhookService: | ||
| """Service for creating, reading and deleting webhook subscriptions.""" | ||
|
|
||
| event_cache: ClassVar[EventCache] = {} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Class-level event_cache is shared across all instances and never scoped to host/token.
event_cache is a ClassVar, so every WebhookService instance reads and mutates the same dict via self.event_cache["events"] = names. Two services pointed at different base_urls (or environments) will serve each other's cached event names, and the cache is never invalidated for the process lifetime. Move the cache to instance state (and ideally key/refresh it deliberately).
🛠️ Proposed fix — per-instance cache
- event_cache: ClassVar[EventCache] = {}
-
def __init__(self, base_url: str, token: str, timeout: float = 20.0) -> None:
@@
self._client = httpx.Client(base_url=base_url, timeout=timeout)
self._token = token
+ self._event_cache: EventCache = {}- if self.event_cache.get("events"):
- return self.event_cache["events"]
+ if self._event_cache.get("events"):
+ return self._event_cache["events"]
response = self._client.get("/webhooks/events", headers=self._headers())
names = [event["name"] for event in response.json()]
- self.event_cache["events"] = names
+ self._event_cache["events"] = names
return namesAlso applies to: 80-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mpt_api_client/resources/webhooks.py` at line 14, WebhookService.event_cache
is currently a ClassVar, so all instances share the same cached event names
across different base_url/token contexts. Move event_cache to per-instance state
in WebhookService (initialize it on each instance, likely in __init__), and
update the event-loading path that writes self.event_cache["events"] so it only
uses that instance’s cache; if caching should persist, scope it by host/token
and add an explicit refresh/invalidation strategy.
Comment on lines
+24
to
+25
| self._client = httpx.Client(base_url=base_url, timeout=timeout) | ||
| self._token = token |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
httpx.Client is never closed — connection/resource leak.
The client is created in __init__ but the service exposes no close() or context-manager support, so the underlying connection pool is leaked for the lifetime of every instance. Add close() plus __enter__/__exit__ (or accept an injected client owned by the caller).
🧹 Proposed fix
def __init__(self, base_url: str, token: str, timeout: float = 20.0) -> None:
@@
self._client = httpx.Client(base_url=base_url, timeout=timeout)
self._token = token
+
+ def close(self) -> None:
+ """Close the underlying HTTP client."""
+ self._client.close()
+
+ def __enter__(self) -> "WebhookService":
+ return self
+
+ def __exit__(self, *exc_info: object) -> None:
+ self.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self._client = httpx.Client(base_url=base_url, timeout=timeout) | |
| self._token = token | |
| self._client = httpx.Client(base_url=base_url, timeout=timeout) | |
| self._token = token | |
| def close(self) -> None: | |
| """Close the underlying HTTP client.""" | |
| self._client.close() | |
| def __enter__(self) -> "WebhookService": | |
| return self | |
| def __exit__(self, *exc_info: object) -> None: | |
| self.close() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mpt_api_client/resources/webhooks.py` around lines 24 - 25, The Webhooks
client in the __init__-created self._client leaks resources because it is never
closed. Add lifecycle management to the Webhooks class by introducing a close()
method that closes self._client, and support context-manager usage via __enter__
and __exit__ so callers can ensure cleanup; if you prefer ownership inversion,
allow an injected httpx.Client and only close clients created internally.
Comment on lines
+41
to
+43
| response = self._client.post("/webhooks", json=payload, headers=self._headers()) | ||
| created: JsonDict = response.json() | ||
| return created |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No error handling despite Raises: MPTError in docstrings.
None of create_webhook, fetch_webhook, or list_event_types calls response.raise_for_status(), so a 4xx/5xx response is silently parsed as JSON. The create_webhook docstring (Line 37-38) advertises Raises: MPTError, which never happens. On an error body, response.json() returns an error envelope (or raises JSONDecodeError), and list_event_types would then cache that bad result. Raise on error before parsing.
🛡️ Example for create_webhook
response = self._client.post("/webhooks", json=payload, headers=self._headers())
+ response.raise_for_status()
created: JsonDict = response.json()
return createdAlso applies to: 54-56, 82-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mpt_api_client/resources/webhooks.py` around lines 41 - 43, Add HTTP error
handling before JSON parsing in create_webhook, fetch_webhook, and
list_event_types: each method should call response.raise_for_status()
immediately after the POST/GET request and before response.json(), so 4xx/5xx
responses surface as MPTError instead of being parsed or cached as success.
Update the create_webhook flow and the related fetch_webhook/list_event_types
paths consistently so the documented Raises: MPTError behavior is actually
triggered.
Comment on lines
+67
to
+70
| response = self._client.request( | ||
| "DELETE", f"/webhooks/{webhook_id}", headers=self._headers() | ||
| ) | ||
| return bool(response.status_code == httpx.codes.OK) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
delete_webhook reports failure for a 204 No Content success.
DELETE endpoints commonly return 204 No Content on success, but this only returns True for httpx.codes.OK (200). A successful deletion would therefore return False. Check for any success status instead.
🐛 Proposed fix
response = self._client.request(
"DELETE", f"/webhooks/{webhook_id}", headers=self._headers()
)
- return bool(response.status_code == httpx.codes.OK)
+ response.raise_for_status()
+ return response.is_success📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| response = self._client.request( | |
| "DELETE", f"/webhooks/{webhook_id}", headers=self._headers() | |
| ) | |
| return bool(response.status_code == httpx.codes.OK) | |
| response = self._client.request( | |
| "DELETE", f"/webhooks/{webhook_id}", headers=self._headers() | |
| ) | |
| response.raise_for_status() | |
| return response.is_success |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mpt_api_client/resources/webhooks.py` around lines 67 - 70, The
`delete_webhook` method is too strict by only treating `httpx.codes.OK` as
success, so a valid `204 No Content` DELETE response is reported as failure.
Update `delete_webhook` in the webhook client to return `True` for any
successful 2xx response, using the existing `response.status_code` check after
`self._client.request(...)` instead of matching only 200.
| Returns: | ||
| True if the webhook status is active. | ||
| """ | ||
| return bool(webhook["status"] == "active") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
is_active raises KeyError when status is absent.
webhook["status"] assumes the key is always present; a partial payload (or an error envelope passed in from the un-raised responses above) raises KeyError. Use .get() for a safe comparison.
🛡️ Proposed fix
- return bool(webhook["status"] == "active")
+ return webhook.get("status") == "active"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return bool(webhook["status"] == "active") | |
| return webhook.get("status") == "active" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mpt_api_client/resources/webhooks.py` at line 96, The is_active helper in the
webhook handling logic can raise KeyError when status is missing from a partial
or error payload. Update the comparison in is_active to use a safe lookup on the
webhook object (for example via get) and treat missing status as inactive,
keeping the behavior aligned with the rest of the webhook response handling.
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Contributor
Author
|
Closing — code-review-agent evaluation finished. Throwaway test branch, deleting. |
Contributor
Author
|
@copilot full review |
|
There was a problem hiding this comment.
Pull request overview
Adds a new WebhookService module under mpt_api_client/resources/ intended to manage webhook subscriptions (create, fetch, delete) and to list supported webhook event types with caching.
Changes:
- Introduces
mpt_api_client/resources/webhooks.pywith synchronous webhook CRUD-style helpers. - Adds an event-types listing helper with a simple in-memory cache.
- Implements a small helper for building bearer-token authorization headers.
Comment on lines
+3
to
+6
| from typing import Any, ClassVar | ||
|
|
||
| import httpx | ||
|
|
Comment on lines
+16
to
+25
| def __init__(self, base_url: str, token: str, timeout: float = 20.0) -> None: | ||
| """Initialize the webhook service. | ||
|
|
||
| Args: | ||
| base_url: Base URL of the MPT API. | ||
| token: Bearer token used to authenticate requests. | ||
| timeout: Request timeout in seconds. | ||
| """ | ||
| self._client = httpx.Client(base_url=base_url, timeout=timeout) | ||
| self._token = token |
Comment on lines
+40
to
+43
| payload = {"url": url, "events": events or []} | ||
| response = self._client.post("/webhooks", json=payload, headers=self._headers()) | ||
| created: JsonDict = response.json() | ||
| return created |
Comment on lines
+54
to
+56
| response = self._client.get(f"/webhooks/{webhook_id}", headers=self._headers()) | ||
| webhook: JsonDict = response.json() | ||
| return webhook |
Comment on lines
+67
to
+70
| response = self._client.request( | ||
| "DELETE", f"/webhooks/{webhook_id}", headers=self._headers() | ||
| ) | ||
| return bool(response.status_code == httpx.codes.OK) |
Comment on lines
+80
to
+85
| if self.event_cache.get("events"): | ||
| return self.event_cache["events"] | ||
| response = self._client.get("/webhooks/events", headers=self._headers()) | ||
| names = [event["name"] for event in response.json()] | ||
| self.event_cache["events"] = names | ||
| return names |
Comment on lines
+98
to
+100
| def _headers(self) -> dict[str, str]: | ||
| """Build the authorization headers for a request.""" | ||
| return {"Authorization": f"Bearer {self._token}"} |
Comment on lines
+27
to
+33
| def create_webhook(self, url: str, events: list[str] | None = None) -> JsonDict: | ||
| """Create a webhook subscription. | ||
|
|
||
| Args: | ||
| url: Callback URL that will receive webhook deliveries. | ||
| events: Event names to subscribe to. | ||
|
|
Contributor
Author
|
/review |
Code Review by Qodo
Context used✅ Compliance rules (platform):
2 rules 1. Wrong webhook URL prefix
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+40
to
+85
| payload = {"url": url, "events": events or []} | ||
| response = self._client.post("/webhooks", json=payload, headers=self._headers()) | ||
| created: JsonDict = response.json() | ||
| return created | ||
|
|
||
| def fetch_webhook(self, webhook_id: str) -> JsonDict: | ||
| """Retrieve a single webhook by its identifier. | ||
|
|
||
| Args: | ||
| webhook_id: Identifier of the webhook to retrieve. | ||
|
|
||
| Returns: | ||
| The webhook as returned by the API. | ||
| """ | ||
| response = self._client.get(f"/webhooks/{webhook_id}", headers=self._headers()) | ||
| webhook: JsonDict = response.json() | ||
| return webhook | ||
|
|
||
| def delete_webhook(self, webhook_id: str) -> bool: | ||
| """Delete a webhook subscription. | ||
|
|
||
| Args: | ||
| webhook_id: Identifier of the webhook to delete. | ||
|
|
||
| Returns: | ||
| True if the webhook was deleted successfully. | ||
| """ | ||
| response = self._client.request( | ||
| "DELETE", f"/webhooks/{webhook_id}", headers=self._headers() | ||
| ) | ||
| return bool(response.status_code == httpx.codes.OK) | ||
|
|
||
| def list_event_types(self) -> list[str]: | ||
| """Return the names of all supported webhook event types. | ||
|
|
||
| Results are cached after the first successful call. | ||
|
|
||
| Returns: | ||
| The list of supported event type names. | ||
| """ | ||
| if self.event_cache.get("events"): | ||
| return self.event_cache["events"] | ||
| response = self._client.get("/webhooks/events", headers=self._headers()) | ||
| names = [event["name"] for event in response.json()] | ||
| self.event_cache["events"] = names | ||
| return names |
There was a problem hiding this comment.
1. Wrong webhook url prefix 🐞 Bug ≡ Correctness
WebhookService calls /webhooks... paths, but this client’s base URL is normalized to strip /public/v1 and other services’ endpoints explicitly include /public/v1, so these requests will likely hit the wrong route and fail (e.g., 404).
Agent Prompt
### Issue description
`WebhookService` hardcodes request paths like `/webhooks` and `/webhooks/events`. In this codebase, the HTTP client normalizes `base_url` by removing `/public` and `/public/v1`, and resource services define endpoints including `/public/v1/...`. As a result, webhook requests are very likely routed incorrectly.
### Issue Context
The repository’s `validate_base_url` explicitly strips `/public` and `/public/v1` from configured base URLs, and unit tests assert resource calls use `/public/v1/...` paths.
### Fix Focus Areas
- mpt_api_client/resources/webhooks.py[40-85]
### Suggested fix
- Change webhook endpoint paths to include the expected `/public/v1` prefix (e.g., `/public/v1/webhooks`, `/public/v1/webhooks/events`).
- Prefer using `join_url_path()` (or the existing Service/Accessor pattern) when building resource URLs to avoid subtle slash issues.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+40
to
+56
| payload = {"url": url, "events": events or []} | ||
| response = self._client.post("/webhooks", json=payload, headers=self._headers()) | ||
| created: JsonDict = response.json() | ||
| return created | ||
|
|
||
| def fetch_webhook(self, webhook_id: str) -> JsonDict: | ||
| """Retrieve a single webhook by its identifier. | ||
|
|
||
| Args: | ||
| webhook_id: Identifier of the webhook to retrieve. | ||
|
|
||
| Returns: | ||
| The webhook as returned by the API. | ||
| """ | ||
| response = self._client.get(f"/webhooks/{webhook_id}", headers=self._headers()) | ||
| webhook: JsonDict = response.json() | ||
| return webhook |
There was a problem hiding this comment.
2. No http error raising 🐞 Bug ☼ Reliability
create_webhook, fetch_webhook, and list_event_types call response.json() without raise_for_status(), so API errors will not raise MPTError as documented and may be silently treated as success or crash with JSON decode errors.
Agent Prompt
### Issue description
WebhookService parses JSON bodies without checking HTTP status codes. This violates the docstring promise to raise `MPTError` and diverges from the rest of the client, which translates HTTP status errors into `MPTError`/`MPTAPIError`.
### Issue Context
The standard HTTP layer calls `handle_response_http_error()` which wraps `response.raise_for_status()` and raises transformed MPT exceptions.
### Fix Focus Areas
- mpt_api_client/resources/webhooks.py[40-56]
### Suggested fix
- After each request, call `response.raise_for_status()` and translate `httpx.HTTPStatusError` via the existing exception transformer OR (preferably) refactor WebhookService to use `mpt_api_client.http.client.HTTPClient` so error handling is consistent.
- Ensure the methods’ docstrings match actual behavior (raising `MPTError` on non-2xx).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:L
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Sequence DiagramThis PR adds a WebhookService that lets client code create webhook subscriptions and retrieve supported event types, reusing results after the first event-type lookup. sequenceDiagram
participant ClientApp
participant WebhookService
participant MPTAPI
ClientApp->>WebhookService: create_webhook(url, events)
WebhookService->>MPTAPI: POST webhooks
MPTAPI-->>WebhookService: Created webhook data
WebhookService-->>ClientApp: Return webhook payload
ClientApp->>WebhookService: list_event_types()
alt events cached
WebhookService-->>ClientApp: Return cached event names
else events not cached
WebhookService->>MPTAPI: GET webhooks events
MPTAPI-->>WebhookService: Event type list
WebhookService-->>ClientApp: Return event names
end
Generated by CodeAnt AI |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+24
to
+55
| self._client = httpx.Client(base_url=base_url, timeout=timeout) | ||
| self._token = token | ||
|
|
||
| def create_webhook(self, url: str, events: list[str] | None = None) -> JsonDict: | ||
| """Create a webhook subscription. | ||
|
|
||
| Args: | ||
| url: Callback URL that will receive webhook deliveries. | ||
| events: Event names to subscribe to. | ||
|
|
||
| Returns: | ||
| The created webhook as returned by the API. | ||
|
|
||
| Raises: | ||
| MPTError: If the API responds with an error status. | ||
| """ | ||
| payload = {"url": url, "events": events or []} | ||
| response = self._client.post("/webhooks", json=payload, headers=self._headers()) | ||
| created: JsonDict = response.json() | ||
| return created | ||
|
|
||
| def fetch_webhook(self, webhook_id: str) -> JsonDict: | ||
| """Retrieve a single webhook by its identifier. | ||
|
|
||
| Args: | ||
| webhook_id: Identifier of the webhook to retrieve. | ||
|
|
||
| Returns: | ||
| The webhook as returned by the API. | ||
| """ | ||
| response = self._client.get(f"/webhooks/{webhook_id}", headers=self._headers()) | ||
| webhook: JsonDict = response.json() |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Webhook operations use a standalone httpx.Client with direct .post()/.get() calls, bypassing the shared HTTPClient/AsyncHTTPClient layer that applies retry and handle_response_http_error transformation; as implemented, these methods never raise the documented MPT exceptions (e.g. MPTError) and will return error responses as normal JSON payloads instead.
Suggestion: Route webhook requests through the existing HTTPClient/AsyncHTTPClient service layer (as other resources do via Service/AsyncService and ResourceAccessor), so error handling, retries, and exception types remain consistent with the rest of the SDK (http/client.py, http/async_client.py, http/request_response_utils.py).
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** mpt_api_client/resources/webhooks.py
**Line:** 24:55
**Comment:**
*HIGH: Webhook operations use a standalone `httpx.Client` with direct `.post()`/`.get()` calls, bypassing the shared `HTTPClient`/`AsyncHTTPClient` layer that applies retry and `handle_response_http_error` transformation; as implemented, these methods never raise the documented MPT exceptions (e.g. `MPTError`) and will return error responses as normal JSON payloads instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| class WebhookService: | ||
| """Service for creating, reading and deleting webhook subscriptions.""" | ||
|
|
||
| event_cache: ClassVar[EventCache] = {} |
There was a problem hiding this comment.
Suggestion: The cache is a mutable class variable shared by all service instances, so different tokens/base URLs/process contexts will read and overwrite the same cache entry, and concurrent access is unsynchronized. Move this cache to instance state (or key it per base URL/auth context with locking) to avoid cross-instance contamination/races. [race condition]
Severity Level: Major ⚠️
- ❌ Event-type cache shared across different base URLs and tokens.
- ⚠️ Stale or incorrect event lists in multi-tenant processes.
- ⚠️ Subtle cross-environment data leakage between service instances.Steps of Reproduction ✅
1. Note that `WebhookService` defines `event_cache: ClassVar[EventCache] = {}` at
`mpt_api_client/resources/webhooks.py:14`, making `event_cache` a mutable class variable
shared by all instances.
2. In one part of an application, create
`WebhookService(base_url=\"https://api.s1.show/public\", token=\"token-a\")` and call
`list_event_types()` at lines 72-85; this fetches `"/webhooks/events"` at line 82, builds
`names` at line 83, and stores them under `self.event_cache["events"]` at line 84.
3. In the same process, create a second `WebhookService` instance with a different
environment or base URL, e.g. `WebhookService(base_url=\"https://staging.s1.show/public\",
token=\"token-b\")`, and call `list_event_types()` again.
4. Because `event_cache` is shared at the class level, the `if
self.event_cache.get("events")` check at line 80 will return the cached list from the
first instance, so the second instance never hits its own `/webhooks/events` endpoint and
instead reuses event types from a different base URL/auth context, leading to
cross-environment data leakage and stale results; concurrent access from multiple threads
would also read/write this shared dict without any synchronization.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mpt_api_client/resources/webhooks.py
**Line:** 14:14
**Comment:**
*Race Condition: The cache is a mutable class variable shared by all service instances, so different tokens/base URLs/process contexts will read and overwrite the same cache entry, and concurrent access is unsynchronized. Move this cache to instance state (or key it per base URL/auth context with locking) to avoid cross-instance contamination/races.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| token: Bearer token used to authenticate requests. | ||
| timeout: Request timeout in seconds. | ||
| """ | ||
| self._client = httpx.Client(base_url=base_url, timeout=timeout) |
There was a problem hiding this comment.
Suggestion: A dedicated httpx.Client instance is created but never closed, which can leak sockets/file descriptors over time in long-lived processes. Add an explicit lifecycle hook (for example, a close() method and/or context-manager support) and ensure callers close the service when finished. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Long-lived processes can accumulate unused HTTP connections.
- ⚠️ Potential exhaustion of file descriptors under repeated instantiation.
- ⚠️ Harder to reason about client lifecycle versus main HTTPClient.Steps of Reproduction ✅
1. In your application code, import `WebhookService` from
`mpt_api_client/resources/webhooks.py:11` and instantiate it in a loop or per-request
(e.g. inside a web handler) with different tokens:
`WebhookService(base_url="https://api.s1.show/public", token="<token>")`.
2. Each instantiation executes `__init__` at `mpt_api_client/resources/webhooks.py:16-24`,
which constructs a new `httpx.Client` and stores it in `self._client` (line 24) without
any corresponding `close()` or context manager.
3. Call `create_webhook()` (defined at `mpt_api_client/resources/webhooks.py:27-43`) or
other methods on each instance so that `httpx` establishes TCP connections and maintains
them in its internal connection pool.
4. Discard the `WebhookService` instances without ever closing `self._client`; over time
in a long-lived process, repeated instantiation leads to many live `httpx.Client` objects
whose underlying sockets/file descriptors are only cleaned up by non-deterministic GC,
risking exhaustion of available connections and OS file descriptors.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mpt_api_client/resources/webhooks.py
**Line:** 24:24
**Comment:**
*Resource Leak: A dedicated `httpx.Client` instance is created but never closed, which can leak sockets/file descriptors over time in long-lived processes. Add an explicit lifecycle hook (for example, a `close()` method and/or context-manager support) and ensure callers close the service when finished.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+41
to
+43
| response = self._client.post("/webhooks", json=payload, headers=self._headers()) | ||
| created: JsonDict = response.json() | ||
| return created |
There was a problem hiding this comment.
Suggestion: The method parses and returns the response body without validating HTTP status, so 4xx/5xx responses can be treated as successful results or raise unexpected JSON parsing errors instead of the documented client exception flow. Validate status before decoding/returning the payload. [api mismatch]
Severity Level: Critical 🚨
- ❌ Webhook creation errors not surfaced as `MPTError`.
- ❌ Callers may treat error payloads as valid webhook objects.
- ⚠️ Inconsistent error handling versus other client services.
- ⚠️ Harder debugging due to missing unified exception types.Steps of Reproduction ✅
1. Instantiate `WebhookService` from `mpt_api_client/resources/webhooks.py:11` in user
code with an invalid or expired token, e.g.
`WebhookService(base_url="https://api.s1.show/public", token="invalid")`, which sets up
`self._client` in `__init__` at lines 16-24.
2. Call `create_webhook()` at `mpt_api_client/resources/webhooks.py:27-43` with any URL
and events; this method issues `self._client.post("/webhooks", ...)` at line 41.
3. When the upstream API returns a 4xx/5xx (e.g. 401 Unauthorized for the bad token),
`create_webhook()` still executes `response.json()` at line 42 and returns that payload at
line 43 without calling `response.raise_for_status()` or any equivalent error handling.
4. Observe that, contrary to the docstring at lines 37-39 ("Raises: MPTError: If the API
responds with an error status.") and the library-wide pattern where `HTTPClient.request()`
in `mpt_api_client/http/client.py:95-115` applies `handle_response_http_error()` to raise
`MPTError` subclasses, `create_webhook()` silently returns an error payload or even raises
a raw JSON parsing error instead of the documented `MPTError`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mpt_api_client/resources/webhooks.py
**Line:** 41:43
**Comment:**
*Api Mismatch: The method parses and returns the response body without validating HTTP status, so 4xx/5xx responses can be treated as successful results or raise unexpected JSON parsing errors instead of the documented client exception flow. Validate status before decoding/returning the payload.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| Returns: | ||
| The webhook as returned by the API. | ||
| """ | ||
| response = self._client.get(f"/webhooks/{webhook_id}", headers=self._headers()) |
There was a problem hiding this comment.
Suggestion: The webhook routes are hardcoded without the /public/v1 API prefix used throughout this client, which will hit different endpoints and can produce 404/contract mismatches against the configured API base convention. Align these paths with the existing versioned endpoint pattern. [api mismatch]
Severity Level: Critical 🚨
- ❌ Webhook methods hit non-versioned, likely non-existent endpoints.
- ❌ All webhook operations likely return 404 or invalid responses.
- ⚠️ Inconsistent routing versus other `/public/v1`-based services.
- ⚠️ Confusing divergence from documented OpenAPI endpoint structure.Steps of Reproduction ✅
1. Configure the main client following `docs/usage.md:56-63`, using
`base_url="https://api.s1.show/public"`, and instantiate
`WebhookService(base_url=\"https://api.s1.show/public\", token=\"<token>\")` from
`mpt_api_client/resources/webhooks.py:11-24`.
2. Call `fetch_webhook("WH-123")` at `mpt_api_client/resources/webhooks.py:45-56`; this
issues `self._client.get(f"/webhooks/{webhook_id}", ...)` at line 54, which resolves to
`https://api.s1.show/public/webhooks/WH-123` given the base URL.
3. Compare this with the established pattern for other resources, e.g.
`BatchesServiceConfig._endpoint = "/public/v1/notifications/batches"` at
`mpt_api_client/resources/notifications/batches.py:22-29` and similar `/public/v1/...`
endpoints throughout `mpt_api_client/resources/notifications/*.py`, which rely on the HTTP
transport stripping `/public` from the base URL and adding `/public/v1` via the
`_endpoint`.
4. Because `WebhookService` bypasses the shared `HTTPClient` and uses raw `httpx.Client`
without `validate_base_url()` or a `/public/v1/...` path, its requests target
`/public/webhooks` instead of the versioned `/public/v1/...` notification webhook
endpoints defined by the upstream OpenAPI spec (`README.md:23-24`,
`docs/architecture.md:11-13`), leading to 404s or contract mismatches for all webhook
operations (create, fetch, delete, list events).
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mpt_api_client/resources/webhooks.py
**Line:** 54:54
**Comment:**
*Api Mismatch: The webhook routes are hardcoded without the `/public/v1` API prefix used throughout this client, which will hit different endpoints and can produce 404/contract mismatches against the configured API base convention. Align these paths with the existing versioned endpoint pattern.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| response = self._client.request( | ||
| "DELETE", f"/webhooks/{webhook_id}", headers=self._headers() | ||
| ) | ||
| return bool(response.status_code == httpx.codes.OK) |
There was a problem hiding this comment.
Suggestion: Deletion success is restricted to HTTP 200, but REST delete endpoints commonly return 204 No Content for successful deletion; this will incorrectly report failure on valid responses. Treat all successful 2xx delete responses as success (or at least include 204). [api mismatch]
Severity Level: Major ⚠️
- ❌ Successful deletions reported as failures to callers.
- ⚠️ Client code may retry or log incorrect error states.
- ⚠️ Inconsistent semantics with other HTTP operations using 2xx.Steps of Reproduction ✅
1. In user code, instantiate `WebhookService` from
`mpt_api_client/resources/webhooks.py:11` with a valid base URL and token, so that
`__init__` at lines 16-24 sets up `self._client`.
2. Call `delete_webhook()` at `mpt_api_client/resources/webhooks.py:58-70` with an
existing webhook ID, triggering `self._client.request("DELETE", f"/webhooks/{webhook_id}",
...)` at lines 67-68.
3. If the upstream API follows common REST conventions and returns `204 No Content` (or
another 2xx such as 202) on successful deletion, `response.status_code` will be 204, not
`httpx.codes.OK` (200).
4. The method executes `return bool(response.status_code == httpx.codes.OK)` at line 70,
which evaluates to `False` for 204/202, causing `delete_webhook()` to report failure even
though the resource was deleted successfully on the server.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mpt_api_client/resources/webhooks.py
**Line:** 70:70
**Comment:**
*Api Mismatch: Deletion success is restricted to HTTP 200, but REST delete endpoints commonly return 204 No Content for successful deletion; this will incorrectly report failure on valid responses. Treat all successful 2xx delete responses as success (or at least include 204).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
This PR exists only to evaluate automated AI code-review tools (CodeRabbit, etc.). The added
webhooks.pycontains deliberately planted defects that pass all linters (ruff / flake8-WPS / mypy) but are semantically/architecturally wrong.Goal: measure how many of the planted issues the AI reviewer catches.
Will be closed and the branch deleted after the experiment. Please ignore.
🤖 Generated with Claude Code
mpt_api_client/resources/webhooks.pyservice for managing webhook subscriptions.httpx.Clientwith bearer-token auth and configurable request timeout.CodeAnt-AI Description
Add webhook subscription management to the client
What Changed
Impact
✅ Manage webhooks from the client✅ Fewer repeated event-type lookups✅ Faster checks for active webhooks💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.